Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix tests from modified get_element_instances #287

Merged
merged 8 commits into from
Dec 26, 2024

Conversation

giovp
Copy link
Member

@giovp giovp commented Jul 9, 2024

folllow scverse/spatialdata#621 and should be merged after that.

I modified couple of plots that I think it made sense, but for two, specifically the NotebookTransformation ones for rotation and affine I did not, you can see below the new version (left) v. old version (right)

affine
image

rotation
image

you can see that the only thing that really changes is the background color, I wonder if it is a matplolib version problem, as I don't think spatialdata#621 impacts that. Any idea @melonora @timtreis ?

@melonora
Copy link
Contributor

melonora commented Jul 9, 2024

yeah this is a known issue, matplotlib is producing different results dependent on both version and platform.

@giovp
Copy link
Member Author

giovp commented Jul 9, 2024

ok so the tests should pass for those two 🤞

@melonora
Copy link
Contributor

melonora commented Jul 9, 2024

hmm this label_categorical color was wrong and is still wrong. Given that it was broken I am ok with this PR, but we need to fix it. What is indicated as being C is actually background label. @timtreis I vaguely remember you workin on a fix for this. Am I correct?

@giovp
Copy link
Member Author

giovp commented Jul 9, 2024

I'm afraid the tests failing are the ones of the figures I have added, I wonder if the reason is precisely this version differing behaviour. Increase tolerance ? or someone has an ubuntu machine to recreate figures?

@melonora
Copy link
Contributor

melonora commented Jul 9, 2024

Increasing tolerance will not help as this does not account for large difference in colors which happens with different color for the labels or background.

@timtreis
Copy link
Member

timtreis commented Jul 9, 2024

Yeah, no idea why this is happening. Noticed it in #259 originally but I still have no idea what's causing it. Local to me it looks fine. For this case, I'm just using the image of the runner itself.

@giovp
Copy link
Member Author

giovp commented Jul 10, 2024

Good point, uploaded the new ones from the runner artifacts

@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.76%. Comparing base (d43d3e7) to head (7935a44).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #287   +/-   ##
=======================================
  Coverage   83.76%   83.76%           
=======================================
  Files           8        8           
  Lines        1694     1694           
=======================================
  Hits         1419     1419           
  Misses        275      275           

@giovp
Copy link
Member Author

giovp commented Jul 10, 2024

wtf how can 3.9 and 3.10 not be consistent?

@melonora
Copy link
Contributor

Because matplotlib. Had this before, in these cases for now if this happens we accept the PR

@giovp
Copy link
Member Author

giovp commented Jul 10, 2024

mmh but this is a bit suspicious, like the results seems to put labels with different orders, despite everything being generated by RNG. In the last commit, I took artefacts from 3.9 and copied it to 3.10 and still now both fails

@timtreis timtreis self-assigned this Jul 10, 2024
@melonora
Copy link
Contributor

@timtreis are you still planning to pick this up? Otherwise I can.

@timtreis
Copy link
Member

I think I tightened the GH actions enough so that 3.9 and 3.10 are now consistent. At least I didn't run into inconsistencies in the few last PRs. We still get the black bg on some of the raccoons, only on the runner though.

Can this be closed?

@LucaMarconato
Copy link
Member

I checked the code and the one from Giovanni is correct. The max of the labels is 5, but the labels are non-contiguous, so using len(get_element_instances()) is the way to go.

@LucaMarconato
Copy link
Member

LucaMarconato commented Dec 26, 2024

As pointed out by @melonora, the plots from this commit 42a4ee6 were still wrong as the background should have been black and not colored.

The reason was this line here:

adata.obs["instance_id"] = list(range(adata.n_obs))
which was replacing the new correct value for the instance_id column, with a wrong one, starting from zero.

I have corrected this, verified the consistency with napari-spatialdata and regenerated the ground-truth plot. Now it's ready to merge.

@LucaMarconato LucaMarconato merged commit 49d1893 into main Dec 26, 2024
5 checks passed
@LucaMarconato LucaMarconato deleted the giovp/get_element_instances branch December 26, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants